Skip to content

Data is lost for joined DataBuffer in DataBufferUtils#36714

Merged
bclozel merged 2 commits into
spring-projects:7.0.xfrom
cookie-meringue:fix-data-loss-in-databufferutils
Jun 4, 2026
Merged

Data is lost for joined DataBuffer in DataBufferUtils#36714
bclozel merged 2 commits into
spring-projects:7.0.xfrom
cookie-meringue:fix-data-loss-in-databufferutils

Conversation

@cookie-meringue
Copy link
Copy Markdown
Contributor

Summary

Fixes a bug in DataBufferUtils.write(Publisher<DataBuffer>, WritableByteChannel) where only the first ByteBuffer is written and the remaining data is silently discarded when writing a DataBuffer composed of multiple NIO ByteBuffers to a synchronous channel.

Conditions & Root Cause

The core of this bug lies in a violation of the DataBuffer.ByteBufferIterator contract, rather than a flaw in a specific data structure.

When writing data to a channel, the WritableByteChannelSubscriber.hookOnNext() method in DataBufferUtils.write() operates as follows:

// DataBufferUtils.WritableByteChannelSubscriber.hookOnNext
@Override
protected void hookOnNext(DataBuffer dataBuffer) {
    try {
        try (DataBuffer.ByteBufferIterator iterator = dataBuffer.readableByteBuffers()) {
            ByteBuffer byteBuffer = iterator.next(); // Fetches only the first element.
            while (byteBuffer.hasRemaining()) {
                this.channel.write(byteBuffer);
            }
        } // Remaining ByteBuffers are never accessed and become unreachable on close.
        this.sink.next(dataBuffer);
        request(1);
    }
    catch (IOException ex) { ... }
} 

According to the DataBuffer's Javadoc, the iterator returned by dataBuffer.readableByteBuffers() exposes each ByteBuffer, and the caller is responsible for iterating through them all using while (iterator.hasNext()).

// DataBuffer
/**
 * Returns a closeable iterator over each {@link ByteBuffer} in this data
 * buffer that can be read.
 * ...
 */
ByteBufferIterator readableByteBuffers();

However, the current WritableByteChannelSubscriber.hookOnNext() calls iterator.next() exactly once without an outer loop.

Consequently, if the iterator exposes two or more elements (N > 1), the second and all subsequent ByteBuffers are never accessed.

They are released and become inaccessible as the try-with-resources block closes, resulting in silent data loss.

Fix

Add the missing outer while (iterator.hasNext()) loop in WritableByteChannelSubscriber#hookOnNext so that all ByteBuffers exposed by the iterator are written:

@Override
protected void hookOnNext(DataBuffer dataBuffer) {
    try {
        try (DataBuffer.ByteBufferIterator iterator = dataBuffer.readableByteBuffers()) {
            while (iterator.hasNext()) {                    // added
                ByteBuffer byteBuffer = iterator.next();
                while (byteBuffer.hasRemaining()) {
                    this.channel.write(byteBuffer);
                }
            }
        }
        this.sink.next(dataBuffer);
        request(1);
    }
    catch (IOException ex) { ... }
}

Tests

Added writeWritableByteChannelWithJoinedBuffer to DataBufferUtilsTests. The test joins two DataBuffers via bufferFactory.join(List.of(foo, bar)), writes the resulting DataBuffer to a synchronous channel, and asserts the file contains "foobar".

Background

This bug was introduced during the refactoring in commit 3e2f58c

During the migration from dataBuffer.toByteBuffer() (which guaranteed a single buffer) to dataBuffer.readableByteBuffers() (which exposes multiple buffers) for zero-copy optimization, the existing single-buffer processing pattern of the synchronous write path was left unchanged, omitting the necessary outer iteration loop.

Notably, the asynchronous sibling WriteCompletionHandler (in the same file, migrated in the same commit) iterates correctly via its callback chain — further suggesting that the missing loop in the synchronous path is an oversight rather than intent.

Existing callers follow the iterator contract

All other callers of readableByteBuffers() in the codebase iterate correctly using the while (iterator.hasNext()) pattern:

  • JacksonTokenizer
  • Jackson2Tokenizer
  • XmlEventDecoder
  • PartGenerator
  • JettyClientHttpRequest
  • ServletServerHttpResponse
  • StandardWebSocketSession

Impact

Multi-element iterators are produced wherever NettyDataBufferFactory.join(List<DataBuffer>) is invoked, which is the standard way Spring combines multiple DataBuffers into one. This is used pervasively across WebFlux:

  • Body codecs (DataBufferUtils.join to buffer the entire body before parsing): Jackson JSON/XML/CBOR/Smile, Gson, Kotlin Serialization, Protobuf, Form, Multipart.
  • Body encoders (bufferFactory.join to prepend prefixes/separators): Jackson encoders, Gson, Protobuf JSON.
  • View/resource composition: ViewResolutionResultHandler (fragment joining), CssLinkResourceTransformer, ContentVersionStrategy.

The bug surfaces when the resulting DataBuffer is subsequently written to a synchronous WritableByteChannel. The asynchronous AsynchronousFileChannel path (WriteCompletionHandler) is unaffected.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Apr 27, 2026
@bclozel bclozel added the in: core Issues in core modules (aop, beans, core, context, expression) label Apr 27, 2026
@bclozel bclozel self-assigned this Jun 4, 2026
@bclozel bclozel changed the base branch from main to 7.0.x June 4, 2026 09:31
@bclozel bclozel changed the title Fix multi-buffer data loss in DataBufferUtils.write Data is lost for joined DataBuffer in DataBufferUtils Jun 4, 2026
cookie-meringue and others added 2 commits June 4, 2026 12:17
Prior to this commit, WritableByteChannelSubscriber.hookOnNext() called
iterator.next() exactly once. If a DataBuffer consisted of multiple NIO
ByteBuffers (e.g., NettyDataBuffer wrapping a CompositeByteBuf), only
the first buffer was written to the channel, and the remaining buffers
were silently ignored and lost.

This commit adds the missing while (iterator.hasNext()) outer loop to
ensure all fragmented buffers exposed by the iterator are completely
and safely written to the synchronous channel.

See spring-projectsgh-36714

Signed-off-by: KimDaehyeon <daehyeon3351@gmail.com>
This commit adds further fixes in the same area, since there were
similar bugs in the WriteCompletionHandler:
* databuffers were not always emitted when fully read in the onNext hook
* on completion, the iterator was closed too early, before it was fully
  read
* on completion, writing the next bytebuffers from the iterator would
  always reuse the first one and not update the attachment

Closes spring-projectsgh-36714
@bclozel bclozel force-pushed the fix-data-loss-in-databufferutils branch from a56531d to 80534f9 Compare June 4, 2026 10:18
@bclozel bclozel added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jun 4, 2026
@bclozel bclozel added this to the 7.0.8 milestone Jun 4, 2026
bclozel pushed a commit that referenced this pull request Jun 4, 2026
Prior to this commit, WritableByteChannelSubscriber.hookOnNext() called
iterator.next() exactly once. If a DataBuffer consisted of multiple NIO
ByteBuffers (e.g., NettyDataBuffer wrapping a CompositeByteBuf), only
the first buffer was written to the channel, and the remaining buffers
were silently ignored and lost.

This commit adds the missing while (iterator.hasNext()) outer loop to
ensure all fragmented buffers exposed by the iterator are completely
and safely written to the synchronous channel.

See gh-36714

Signed-off-by: KimDaehyeon <daehyeon3351@gmail.com>
bclozel added a commit that referenced this pull request Jun 4, 2026
This commit adds further fixes in the same area, since there were
similar bugs in the WriteCompletionHandler:
* databuffers were not always emitted when fully read in the onNext hook
* on completion, the iterator was closed too early, before it was fully
  read
* on completion, writing the next bytebuffers from the iterator would
  always reuse the first one and not update the attachment

Closes gh-36714
@bclozel bclozel merged commit 80534f9 into spring-projects:7.0.x Jun 4, 2026
2 of 3 checks passed
@bclozel
Copy link
Copy Markdown
Member

bclozel commented Jun 4, 2026

Thanks for your contribution @cookie-meringue !

@github-actions github-actions Bot added the status: backported An issue that has been backported to maintenance branches label Jun 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

in: core Issues in core modules (aop, beans, core, context, expression) status: backported An issue that has been backported to maintenance branches type: bug A general bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants